Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#54008 Issue Fix #54058

Closed
wants to merge 17 commits into from
Closed

Conversation

j0nimost
Copy link

Passing t.Key (the name of the template).

Passing name of the Template
@ghost
Copy link

ghost commented Jun 11, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Passing t.Key (the name of the template).

Author: j0nimost
Assignees: -
Labels:

area-Extensions-Logging

Milestone: -

@ghost ghost added this to Active PRs in ML, Extensions, Globalization, etc, POD. Jun 11, 2021
@maryamariyan maryamariyan self-requested a review June 11, 2021 12:45
Copy link
Member

@maryamariyan maryamariyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add a line similar to

Assert.Contains("Argument 'foo' is not referenced from the logging message", diagnostics[0].GetMessage(), StringComparison.InvariantCulture);

After

Assert.Equal(DiagnosticDescriptors.TemplateHasNoCorrespondingArgument.Id, diagnostics[0].Id);

to assert the fix.

Added Logger Fix TestCase
@dnfadmin
Copy link

dnfadmin commented Jun 11, 2021

CLA assistant check
All CLA requirements met.

@j0nimost
Copy link
Author

Just added the test

Copy link
Member

@maryamariyan maryamariyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @j0nimost for the PR. The unit test is failing, because the message you are asserting against, is not the one presented in the issue description. (Refer to Expected/Actual presented in issue description: #54008 (comment))

Also refer to PR: #54080

Matching test chase with expected case
@j0nimost
Copy link
Author

Just updated the test

@j0nimost
Copy link
Author

j0nimost commented Jun 14, 2021

I have tried sifting through the 3 failing tests but I can't tell where the problem is. Any Pointers @maryamariyan

What I found:

System.Numerics.Tests.GenericVectorTests.SumDouble [FAIL]
      System.NullReferenceException : Object reference not set to an instance of an object.
      Stack Trace:
        /_/src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs(3176,0): at System.Numerics.Tests.GenericVectorTests.TestSum[T](Func`2 expected)
        /_/src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs(3152,0): at System.Numerics.Tests.GenericVectorTests.SumDouble()
    System.Numerics.Tests.GenericVectorTests.SumSingle [FAIL]
      System.NullReferenceException : Object reference not set to an instance of an object.
      Stack Trace:
        /_/src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs(3176,0): at System.Numerics.Tests.GenericVectorTests.TestSum[T](Func`2 expected)
        /_/src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs(3149,0): at System.Numerics.Tests.GenericVectorTests.SumSingle()

@maryamariyan
Copy link
Member

The test failures are unrelated to your changes and are addressed in #54119

I noticed you added one commit per *.xlf file. When you build the gen\Microsoft.Extensions.Logging.Generators.csproj project, it automatically updated the xlf files for you.

@j0nimost
Copy link
Author

Oh I presumed I had to had code, I can undo all the commits

@maryamariyan
Copy link
Member

Oh I presumed I had to had code, I can undo all the commits

no need to undo.

@j0nimost
Copy link
Author

ok thanks for the clarification

@maryamariyan
Copy link
Member

just wanted to make sure you've tried building locally as well

@maryamariyan
Copy link
Member

Thank you both @jeffl8n and @j0nimost for your contribution PRs (#54080 and #54058) to this repo.
The two PRs are fixing the same issue and with similar diff, I approved both.

I'll merged PR #54080 since it was finalized first.

@maryamariyan
Copy link
Member

@j0nimost look forward to more contributions in the future :)

ML, Extensions, Globalization, etc, POD. automation moved this from Active PRs to Done Jun 14, 2021
@j0nimost
Copy link
Author

Thank you @maryamariyan

@jeffl8n
Copy link
Contributor

jeffl8n commented Jun 14, 2021

Sorry for the redundancy @j0nimost. I missed where you had already started a fix/PR for it when I submitted mine.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants